-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
📖 Document changes to BYO certificates #9585
📖 Document changes to BYO certificates #9585
Conversation
Skipping CI for Draft Pull Request. |
- Introduced function `CollectInfrastructureLogs` at the `ClusterLogCollector` interface in `test/framework/cluster_proxy.go` to allow collecting infrastructure related logs during tests. | ||
- A `GetTypedConfigOwner` function has been added to the `sigs.k8s.io./cluster-api/bootstrap/util` package. It is equivalent to `GetConfigOwner` except that it uses the cached typed client instead of the uncached unstructured client, so `GetTypedConfigOwner` is expected to be more performant. | ||
- `ClusterToObjectsMapper` in `sigs.k8s.io./cluster-api/util` has been deprecated, please use `ClusterToTypedObjectsMapper` instead. | ||
- The generated `kubeconfig` by the Control Plane providers must be labelled with the key-value pair `cluster.x-k8s.io/cluster-name=${CLUSTER_NAME}`. | ||
This is required for the CAPI managers caches to store and retrieve them for the required operations. | ||
This is required for the CAPI managers caches to store and retrieve them for the required operations. | ||
- When using custom certificates, the certificates must be labeled with the key-value pair `cluster.x-k8s.io/cluster-name=${CLUSTER_NAME}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The ${CLUSTER_NAME}
is not really required. As long as the key exists, it will work, but perhaps it is better to be consistent throughout the docs with the name also?
internal/test/envtest/environment.go
Outdated
Cache: cache.Options{ | ||
// Namespaces: watchNamespaces, | ||
SyncPeriod: &syncPeriod, | ||
ByObject: map[client.Object]cache.ByObject{ | ||
// Note: Only Secrets with the cluster name label are cached. | ||
// The default client of the manager won't use the cache for secrets at all (see Client.Cache.DisableFor). | ||
// The cached secrets will only be used by the secretCachingClient we create below. | ||
&corev1.Secret{}: { | ||
Label: clusterSecretCacheSelector, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this from ae9464d#diff-8bdee7925f4d1adc50383754391472636156a2a857a7c76290f6073c8d83f5cd
Not sure if it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems ok, cc @sbueringer for a double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/area documentation
65a4050
to
2609cd2
Compare
/test pull-cluster-api-verify-main |
g.Expect(kcp.Status.Replicas).To(BeEquivalentTo(1)) | ||
g.Expect(conditions.IsFalse(kcp, controlplanev1.AvailableCondition)).To(BeTrue()) | ||
|
||
k, err := kubeconfig.FromSecret(ctx, env, util.ObjectKey(cluster)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should we check if the KubeConfig is actually using the custom CA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the huge delay...
I think it would be a good addition to check the CA, but I'm not sure how to do it. Could you give me some hint?
Do we check the default CA in any place so I could replicate that?
@fabriziopandini
2609cd2
to
422a4cd
Compare
I have removed the tests for now as I don't have time to work on them. Can we merge the documentation as is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I guess this should get cherry-picked?
LGTM label has been added. Git tree hash: 9dec8433547f9afe2ee7cb1a97a56552ca0abe98
|
Yes please! |
/cherry-pick release-1.6 |
@chrischdi: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for documenting this!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick release-1-6 |
@fabriziopandini: once the present PR merges, I will cherry-pick it on top of release-1-6 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-1-5 |
@fabriziopandini: once the present PR merges, I will cherry-pick it on top of release-1-5 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@chrischdi: new pull request created: #10230 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@fabriziopandini: cannot checkout In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@fabriziopandini: cannot checkout In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-1.5 |
@fabriziopandini: new pull request created: #10234 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
This documents a change in requirements for user provided certificates
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #9568